@danieldegrasse : The display format is RGB565, not BGR565. I think there are two mistakes that makes it "works" in practice for NXP display, but make it confused for others, and that impacts the video capture where we forced RGB565 to BGR565 for display.

• The rkxxx panel overlays should declare RGB565, not BGR565.
• The NXP eLCDIF driver should not force BGR565 to RGB565.

Taking the example of RK055HDMIPI4MA01:
https://www.nxp.com/docs/en/data-sheet/RK055MHD091A0-CTG.pdf

I did not test on hardware though.

@danieldegrasse : I did check with both samples/drivers/display and samples/drivers/video/capture (where we used video_sw_generator to generate RGB565 color bar patttern) and see it works correctly.

I will look through Jason's explaination but honestly, I can't understand where and how Zephyr define a RGB565 to become BGR565. If it's an issue with only NXP's displays, it should not be enforced widely in Zephyr.

Oh, just retested. Indeed, with the display sample, the colors are wrong, from top-left to bottom-left the box colors are : R -> B, G -> R, B -> G and the grey box cycling through a set of incorrect colors as you said, sorry for that (I am not too familiar with the display sample, so I didn't check carefully).

But for the video capture sample, the color bar pattern are all correct how do we explain that ?
/* Black, Blue, Red, Purple, Green, Aqua, Yellow, White */

Actually as far as I am concerned, I must say that I didn't notice that it wasn't correct.

Me too. And I think there are people who also made that mistake. When they validated their display with this sample, they see R, G, B colors, see it changes colors and thought that is fine, but not.

So, that's why I think there are displays which are already correct and we should not "fix" them.

My point is we should fix this problem but should not fix blindly / systematically all the displays which is dangerous. We can identify them gradually and fix them in upcomming PRs if detected.

So this is where I fundamentally disagree- what this PR is doing is swapping the meaning of two constants in Zephyr. From my POV that means we should swap all usages of those constants in tree, so if a display is broken it stays broken and if a display is working it continues to work. Why would we knowingly create breakage in displays (which this PR will do), versus keeping things working as they are.

We know what will break, we are swapping the meaning of two constants. Swap the usages of the constants in tree and things won't break.

Sorry but I disagree here too. In fact, we don't know which will break unless we test all the displays (which will be difficult).
This fix is actually not a swap of the usage of two constant. We only see that the video sample, the display sample and the NXP's elcdif driver made the mistake so we fixed it.

The assumption that all the in-tree displays are based on the display sample for validation and needs a swap is strong but I am sure that not all of them (LTDC is an example and some other boards and display drivers I see still consider RGB565 is RGB565 and BGR565 is BGR565 so no reason to swap them).

@danieldegrasse thank you for the prompt reply. I did not intend to bypass your comment and it seems I misinterpreted your latest message as for confirmation as it was intermediate step only.

Should the milestone be moved to 4.3.0 or is this considered a bugfix that can land after the feature-freeze?

@danieldegrasse thank you for the prompt reply. I did not intend to bypass your comment and it seems I misinterpreted your latest message as for confirmation as it was intermediate step only.

Should the milestone be moved to 4.3.0 or is this considered a bugfix that can land after the feature-freeze?

No worries! Personally I'd rather see it go in on 4.3.0- I know the naming of the constants is confusing as is (and I'd like to see them changed) but I don't see the naming as a bug

Actually as far as I am concerned, I must say that I didn't notice that it wasn't correct.

Me too. And I think there are people who also made that mistake. When they validated their display with this sample, they see R, G, B colors, see it changes colors and thought that is fine, but not.
So, that's why I think there are displays which are already correct and we should not "fix" them.
My point is we should fix this problem but should not fix blindly / systematically all the displays which is dangerous. We can identify them gradually and fix them in upcomming PRs if detected.

So this is where I fundamentally disagree- what this PR is doing is swapping the meaning of two constants in Zephyr. From my POV that means we should swap all usages of those constants in tree, so if a display is broken it stays broken and if a display is working it continues to work. Why would we knowingly create breakage in displays (which this PR will do), versus keeping things working as they are.

@danieldegrasse what is the definition of "working/not working"?

I think the root question is whether there is agreement here on if these defines are correct or not?

If they are incorrect then we should fix them instead of maintaining an incorrect implementation/usage. If that results in other displays behaving incorrectly they can be fixed.

Actually as far as I am concerned, I must say that I didn't notice that it wasn't correct.

Me too. And I think there are people who also made that mistake. When they validated their display with this sample, they see R, G, B colors, see it changes colors and thought that is fine, but not.
So, that's why I think there are displays which are already correct and we should not "fix" them.
My point is we should fix this problem but should not fix blindly / systematically all the displays which is dangerous. We can identify them gradually and fix them in upcomming PRs if detected.

So this is where I fundamentally disagree- what this PR is doing is swapping the meaning of two constants in Zephyr. From my POV that means we should swap all usages of those constants in tree, so if a display is broken it stays broken and if a display is working it continues to work. Why would we knowingly create breakage in displays (which this PR will do), versus keeping things working as they are.

@danieldegrasse what is the definition of "working/not working"?

I think the root question is whether there is agreement here on if these defines are correct or not?

If they are incorrect then we should fix them instead of maintaining an incorrect implementation/usage. If that results in other displays behaving incorrectly they can be fixed.

The defines are not in alignment with what other projects use- so IMO yes, they are incorrect. However they do work, they just should be changed. Personally I feel this is somewhat similar to a documentation issue, IE something that isn't breaking anyone's use of Zephyr right now. My concern with making the change close to release is that it is difficult to test all displays (as @ngphibang pointed out). IMO the way to make this change is to swap all usages in tree and do it earlier in the release cycle, so that users have some time to notice any issues we might not have anticipated. Swapping all usages (IMO) is lower risk than making the change knowing it will break some displays, but it still isn't zero risk

Well the PR has been posted since October of last year ;-).

But if you are okay, maybe we schedule this to be merged right after the v4.2 release. Then that would allow the next release cycle for users to identify and address any additional displays that need to be changed.

However they do work, they just should be changed. Personally I feel this is somewhat similar to a documentation issue, IE something that isn't breaking anyone's use of Zephyr right now.

When we say "it works", it mean to work only in the world of the display sample. It's a burden to take care of whenver we do RGB565 or BGR565 outside of the display sample or when we go from and to the display domain, e.g. when developing a new multimedia library.

I am okay to let it be till 4.3, or more but I think I will not blindly swap RGB formats for all the in-tree displays without testing it or at least someone helps to point out that it works / breaks on a display.

I think there are people who also made that mistake. When they validated their display with this sample, they see R, G, B colors, see it changes colors and thought that is fine, but not.

So, that's why I think there are displays which are already correct and we should not "fix" them.

@ngphibang I agree, that was my experience when testing many STM32 boards with displays. To me, what is in this PR rn is the correct way forward, but it should be merged in the next release to leave room for potential fixes and more testing.

So this is where I fundamentally disagree- what this PR is doing is swapping the meaning of two constants in Zephyr. From my POV that means we should swap all usages of those constants in tree, so if a display is broken it stays broken and if a display is working it continues to work.

@danieldegrasse I disagree, in my experience when I was testing displays with the sample, I was not paying attention to colors, I noticed the weird flicking of the grey box, but ignored it (similar to what @avolmat-st described), LVGL samples were the way I would actually validate colors and other things...

I think there are people who also made that mistake. When they validated their display with this sample, they see R, G, B colors, see it changes colors and thought that is fine, but not.

So, that's why I think there are displays which are already correct and we should not "fix" them.

@ngphibang I agree, that was my experience when testing many STM32 boards with displays. To me, what is in this PR rn is the correct way forward, but it should be merged in the next release to leave room for potential fixes and more testing.

So this is where I fundamentally disagree- what this PR is doing is swapping the meaning of two constants in Zephyr. From my POV that means we should swap all usages of those constants in tree, so if a display is broken it stays broken and if a display is working it continues to work.

@danieldegrasse I disagree, in my experience when I was testing displays with the sample, I was not paying attention to colors, I noticed the weird flicking of the grey box, but ignored it (similar to what @avolmat-st described), LVGL samples were the way I would actually validate colors and other things...

Hmm ok- thank you for the alternate perspective here. Maybe more displays than I think have this issue. Perhaps when we merge this PR we add a note to the display sample stating what an incorrect color definition looks like?

I'm going to clear my block then, but I agree- I would really rather hold this until the next release and correct displays over a release cycle

i dont think it should be swapped in the sample... Higher chances displays match the current sample correctly than they dont.

@danieldegrasse Is there a possibility that this PR is merged for v4.2.0.RC2 ?
Reason for asking is because STM32N6 capture sample is currently broken and there is no easy way to fix it other than this PR.
Also consensus is that this PR is fixing what is a real issue that should be fixed, so why not starting right now ?
(I know there might be out of tree users, but this can be documented in migration doc)

@danieldegrasse Is there a possibility that this PR is merged for v4.2.0.RC2 ? Reason for asking is because STM32N6 capture sample is currently broken and there is no easy way to fix it other than this PR. Also consensus is that this PR is fixing what is a real issue that should be fixed, so why not starting right now ? (I know there might be out of tree users, but this can be documented in migration doc)

Wouldn't it be possible to fix the STM32 display driver by aligning it with the (backwards) way that Zephyr currently reports display formats? IE something like the following change:

diff --git a/drivers/display/display_stm32_ltdc.c b/drivers/display/display_stm32_ltdc.c
index b34975f8c58..cdd0c63bae3 100644
--- a/drivers/display/display_stm32_ltdc.c
+++ b/drivers/display/display_stm32_ltdc.c
@@ -55,7 +55,7 @@ LOG_MODULE_REGISTER(display_stm32_ltdc, CONFIG_DISPLAY_LOG_LEVEL);
 #elif CONFIG_STM32_LTDC_RGB565
 #define STM32_LTDC_INIT_PIXEL_SIZE     2u
 #define STM32_LTDC_INIT_PIXEL_FORMAT   LTDC_PIXEL_FORMAT_RGB565
-#define DISPLAY_INIT_PIXEL_FORMAT      PIXEL_FORMAT_RGB_565
+#define DISPLAY_INIT_PIXEL_FORMAT      PIXEL_FORMAT_BGR_565
 #else
 #error "Invalid LTDC pixel format chosen"
 #endif
@@ -110,9 +110,9 @@ static int stm32_ltdc_set_pixel_format(const struct device *dev,
        struct display_stm32_ltdc_data *data = dev->data;

        switch (format) {
-       case PIXEL_FORMAT_RGB_565:
+       case PIXEL_FORMAT_BGR_565:
                err = HAL_LTDC_SetPixelFormat(&data->hltdc, LTDC_PIXEL_FORMAT_RGB565, 0);
-               data->current_pixel_format = PIXEL_FORMAT_RGB_565;
+               data->current_pixel_format = PIXEL_FORMAT_BGR_565;
                data->current_pixel_size = 2u;
                break;
        case PIXEL_FORMAT_RGB_888:
@@ -158,7 +158,7 @@ static void stm32_ltdc_get_capabilities(const struct device *dev,
                                     data->hltdc.LayerCfg[0].WindowY0;
        capabilities->supported_pixel_formats = PIXEL_FORMAT_ARGB_8888 |
                                        PIXEL_FORMAT_RGB_888 |
-                                       PIXEL_FORMAT_RGB_565;
+                                       PIXEL_FORMAT_BGR_565;
        capabilities->screen_info = 0;

This aligns with the way the NXP LCDIF currently reports display pixel formats, and I believe that display works with the video sample (although I lack hardware to test)

I went and did some check, current 'RGB565' order is good if we talk about data order, for example, for ssd1351, which behaves correctly with the current code (I checked today and did check with the sample README back when i wrote that driver):

but in memory, it is the opposite:

What I mean is that which is the right way depends on which PoV is taken, and probably varies between devices.

In the end it can be rightfully assumed that most drivers match the sample as it has been, and there is no reason to swap all of them, but rather the offending drivers should be fixed to be the right way relative to what is right now, as a 'proper' way doesnt really exists.

Hi @VynDragon,

Thank you for the double check of the ssd1351 display. I couldn't find an in-tree device tree that uses this display so I suppose you tested on an out-of-tree board ? Maybe there is a chance that the RGB color is already swapped, i.e. reg-map's A[2] property is set to 1 ?
https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/display/solomon%2Cssd1351.yaml#L69

But I believe this is not the case. So, could you help to fill the ssd1351 row with status "need to be fixed" in the table of the issue : #92343 ?

Up to now, we know that there is at least 1 display "need to be fixed / swapped" after the PR merged (ssd1351), but without this PR, at least 3 displays need to be "worked around" (LTDC, eLCDIF, eLCDIF nano).

So, as you also pointed out, there is no painless solution. The benefit of this PR is that we will have a consistent defines for RGB565 / BGR565 across Zephyr's subsystems, e.g. video, display, and which is also compliant with most of other projects, especially the Linux kernel

https://www.kernel.org/doc/html/v6.11/userspace-api/media/v4l/pixfmt-rgb.html

PS:

... and there is no reason to swap all of them

Consensus is that we do not swap all of them. We will fix / swap displays / boards that need to be, like the ssd1351.

Hi @VynDragon,

Thank you for the double check of the ssd1351 display. I couldn't find an in-tree device tree that uses this display so I suppose you tested on an out-of-tree board ? Maybe there is a chance that the RGB color is already swapped, i.e. reg-map's A[2] property is set to 1 ? https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/display/solomon%2Cssd1351.yaml#L69

But I believe this is not the case. So, could you help to fill the ssd1351 row with status "need to be fixed" in the table of the issue : #92343 ?

Up to now, we know that there is at least 1 display "need to be fixed / swapped" after the PR merged (ssd1351), but without this PR, at least 3 displays need to be "worked around" (LTDC, eLCDIF, eLCDIF nano).

So, as you also pointed out, there is no painless solution. The benefit of this PR is that we will have a consistent defines for RGB565 / BGR565 across Zephyr's subsystems, e.g. video, display, and which is also compliant with most of other projects, especially the Linux kernel

https://www.kernel.org/doc/html/v6.11/userspace-api/media/v4l/pixfmt-rgb.html

PS:

... and there is no reason to swap all of them

Consensus is that we do not swap all of them. We will fix / swap displays / boards that need to be, like the ssd1351.

At best i can tell zephyr 'RGB565' is the same as linux RGB565


Which corresponds to the input order of SSD1351 as well, which is RGB565, first byte is rrrrrggg, second byte is ggbbbbb.

In fact it all seems to agree with the zephyr RGB565....

At best i can tell zephyr 'RGB565' is the same as linux RGB565

Well, what you call "zephyr" is actually the display sample, which is "fixed" by the PR.

In the very first comments and in the commits, it is shown that a SW generator, a camera sensor that outputs RGB565 display wrong colors on a eLCDIF display that supports RGB565.

Linux at https://www.kernel.org/doc/html/v6.11/userspace-api/media/v4l/pixfmt-rgb.html mentions little-endian, are you sure it is not because that is swapped around between the camera and the display?

EG this: #79996 (comment) works in the opposite way, EG, on linux:

RGB565 in

big-endian machine: GGGBBBBB RRRRRGGG 
little-endian machine: RRRRRGGG GGGBBBBB 

BGR565 in

big-endian machine: GGGRRRRR BBBBBGGG 
little-endian machine: BBBBBGGG GGGRRRRR 

Well, what you call "zephyr" is actually the display sample, which is "fixed" by the PR.

Degrass said the samples defines what it is for zephyr.

Linux at https://www.kernel.org/doc/html/v6.11/userspace-api/media/v4l/pixfmt-rgb.html mentions little-endian, are you sure it is not because that is swapped around between the camera and the display?

EG this: #79996 (comment) works in the opposite way, EG, on linux:

RGB565 in

big-endian machine: GGGBBBBB RRRRRGGG
little-endian machine: RRRRRGGG GGGBBBBB
BGR565 in

big-endian machine: GGGRRRRR BBBBBGGG
little-endian machine: BBBBBGGG GGGRRRRR

This is opposite with what we found and comments above #79996 (comment)

Degrass said the samples defines what it is for zephyr.

About "the display sample" defines what it is for Zephyr, including the video, I have no idea.

NVM about this discussion, it is inverted in LVGL in practice, so I dont understand why, but you are right. Maybe one thing of note is that I need to both invert RGB565 order AND use CONFIG_LV_COLOR_16_SWAP to get the right output with LVGL.

@danieldegrasse Is there a possibility that this PR is merged for v4.2.0.RC2 ? Reason for asking is because STM32N6 capture sample is currently broken and there is no easy way to fix it other than this PR. Also consensus is that this PR is fixing what is a real issue that should be fixed, so why not starting right now ? (I know there might be out of tree users, but this can be documented in migration doc)

Wouldn't it be possible to fix the STM32 display driver by aligning it with the (backwards) way that Zephyr currently reports display formats? IE something like the following change:

diff --git a/drivers/display/display_stm32_ltdc.c b/drivers/display/display_stm32_ltdc.c
index b34975f8c58..cdd0c63bae3 100644
--- a/drivers/display/display_stm32_ltdc.c
+++ b/drivers/display/display_stm32_ltdc.c
@@ -55,7 +55,7 @@ LOG_MODULE_REGISTER(display_stm32_ltdc, CONFIG_DISPLAY_LOG_LEVEL);
 #elif CONFIG_STM32_LTDC_RGB565
 #define STM32_LTDC_INIT_PIXEL_SIZE     2u
 #define STM32_LTDC_INIT_PIXEL_FORMAT   LTDC_PIXEL_FORMAT_RGB565
-#define DISPLAY_INIT_PIXEL_FORMAT      PIXEL_FORMAT_RGB_565
+#define DISPLAY_INIT_PIXEL_FORMAT      PIXEL_FORMAT_BGR_565
 #else
 #error "Invalid LTDC pixel format chosen"
 #endif
@@ -110,9 +110,9 @@ static int stm32_ltdc_set_pixel_format(const struct device *dev,
        struct display_stm32_ltdc_data *data = dev->data;

        switch (format) {
-       case PIXEL_FORMAT_RGB_565:
+       case PIXEL_FORMAT_BGR_565:
                err = HAL_LTDC_SetPixelFormat(&data->hltdc, LTDC_PIXEL_FORMAT_RGB565, 0);
-               data->current_pixel_format = PIXEL_FORMAT_RGB_565;
+               data->current_pixel_format = PIXEL_FORMAT_BGR_565;
                data->current_pixel_size = 2u;
                break;
        case PIXEL_FORMAT_RGB_888:
@@ -158,7 +158,7 @@ static void stm32_ltdc_get_capabilities(const struct device *dev,
                                     data->hltdc.LayerCfg[0].WindowY0;
        capabilities->supported_pixel_formats = PIXEL_FORMAT_ARGB_8888 |
                                        PIXEL_FORMAT_RGB_888 |
-                                       PIXEL_FORMAT_RGB_565;
+                                       PIXEL_FORMAT_BGR_565;
        capabilities->screen_info = 0;

This aligns with the way the NXP LCDIF currently reports display pixel formats, and I believe that display works with the video sample (although I lack hardware to test)

Hi @danieldegrasse,

yes I confirm that with such modification done the LTDC driver, this allows to make the video sample work with the LTDC as well as the display sample giving the right colors when displayed via the LTDC.
Waiting for the discussion about what should be RGB_565 / BGR_565 PIXEL_FORMAT in Zephyr and in order to have right display colors and video sample working on v4.2.0, is it ok to push such patch and get it merged into v4.2.0 ?
After that, if decision is taken to change formats after v4.2.0, we can change again this past v4.2.0 to align it with new definitions.
For information, on STM32 LTDC side, LVGL samples seems to be fine, with right colors (as far as I've seen with the various demo code I tried).

yes I confirm that with such modification done the LTDC driver, this allows to make the video sample work with the LTDC as well as the display sample giving the right colors when displayed via the LTDC.
Waiting for the discussion about what should be RGB_565 / BGR_565 PIXEL_FORMAT in Zephyr and in order to have right display colors and video sample working on v4.2.0, is it ok to push such patch and get it merged into v4.2.0 ?
After that, if decision is taken to change formats after v4.2.0, we can change again this past v4.2.0 to align it with new definitions.
For information, on STM32 LTDC side, LVGL samples seems to be fine, with right colors (as far as I've seen with the various demo code I tried).

Absolutely- this is what I'd prefer. A narrow patch like what I described to fix the LTDC driver now for 4.2, and post 4.2 we will get this PR merged.

yes I confirm that with such modification done the LTDC driver, this allows to make the video sample work with the LTDC as well as the display sample giving the right colors when displayed via the LTDC.
Waiting for the discussion about what should be RGB_565 / BGR_565 PIXEL_FORMAT in Zephyr and in order to have right display colors and video sample working on v4.2.0, is it ok to push such patch and get it merged into v4.2.0 ?
After that, if decision is taken to change formats after v4.2.0, we can change again this past v4.2.0 to align it with new definitions.
For information, on STM32 LTDC side, LVGL samples seems to be fine, with right colors (as far as I've seen with the various demo code I tried).

Absolutely- this is what I'd prefer. A narrow patch like what I described to fix the LTDC driver now for 4.2, and post 4.2 we will get this PR merged.

PR #92483 pushed